Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add Redis configuration for improved memory management #3427

Merged
merged 5 commits into from
Nov 19, 2024

Conversation

Hassanzadeh-sd
Copy link
Contributor

@Hassanzadeh-sd Hassanzadeh-sd commented Nov 13, 2024

As Sentry continues to evolve, effective resource management becomes crucial for maintaining performance and stability. This update includes configurations that will help optimize Redis's memory usage, ensuring that the system runs efficiently under varying loads.

Key Changes:

  • Added maxmemory Directive: Configured Redis to limit its memory usage to a specified size. This prevents excessive memory consumption and helps maintain system stability.
  • Set maxmemory-policy to allkeys-lru: This policy allows Redis to evict the least recently used keys when it reaches the memory limit, ensuring that frequently accessed data remains available while older, less-used data is removed.

Thank you for considering this enhancement to improve Redis's performance within the Sentry ecosystem!

Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for the PR!

Requested a change and left a question. After that I think this is good to merge but I'll also consult our SRE team to make sure.

Finally, if you could add this boilerplate to your PR description, we'd be able to merge at ease: https://github.com/getsentry/self-hosted/blob/master/.github/PULL_REQUEST_TEMPLATE.md

docker-compose.yml Outdated Show resolved Hide resolved
redis/redis.conf Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.01%. Comparing base (be66069) to head (6d33bdc).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3427   +/-   ##
=======================================
  Coverage   99.01%   99.01%           
=======================================
  Files           3        3           
  Lines         203      203           
=======================================
  Hits          201      201           
  Misses          2        2           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got word from a colleague: "For a self hosted Relay itself, that config would be fine."

@BYK BYK changed the title Add Redis Configuration for Improved Memory Management feat: add Redis configuration for improved memory management Nov 19, 2024
@BYK BYK enabled auto-merge (squash) November 19, 2024 11:20
@BYK
Copy link
Member

BYK commented Nov 19, 2024

Note to followers: if the eviction policy kicks in, there might be data loss as Redis is not just used as a cache but also a processing queue/storage.

@BYK BYK merged commit c3814f0 into getsentry:master Nov 19, 2024
12 checks passed
@Hassanzadeh-sd
Copy link
Contributor Author

thanks. @BYK

@mwarkentin
Copy link
Member

In prod our redis are split into many different clusters and instances, so I don't know what the best setting would be for a deployment where everything is running within a single redis. It might be worth considering volatile-lru instead of allkeys-lru:

volatile-lru: Evict the least recently used keys that have the expire field set to true.

The volatile-lru and volatile-random policies are mainly useful when you want to use a single Redis instance for both caching and for a set of persistent keys. However, you should consider running two separate Redis instances in a case like this, if possible.

BYK added a commit that referenced this pull request Nov 19, 2024
BYK added a commit that referenced this pull request Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants